Repository navigation
Enable deploy from url with token - #1928
s-yonkov-yonkov wants to merge 5 commits into
Conversation
…ROSSITXSADEPLOY-3391] Add bearer token support to the deploy-from-URL feature so MTA archives hosted on GitHub (release assets, raw content) can be fetched using a GitHub PAT or GITHUB_TOKEN. Changes: - UserCredentials: add optional `token` field (additive, backward compatible) - Introduce RequestAuthenticator strategy (Bearer, Basic, UrlUserInfo, None) with explicit precedence: token > basic > url-userinfo > none - AuthenticatorResolver injected as a @nAmed Spring bean into DeployFromUrlRemoteClient (replaces inline if/else ladder) - Fix pre-existing audit-log leak: logStartUploadFromUrl now receives the userinfo-stripped URL instead of the raw decoded URL - Tests: AuthenticatorResolverTest (9), DeployFromUrlRemoteClientTest (+2), FilesApiServiceImplTest (+1 audit-log regression test) # Conflicts: # multiapps-controller-web/src/main/java/org/cloudfoundry/multiapps/controller/web/upload/client/DeployFromUrlRemoteClient.java # multiapps-controller-web/src/test/java/org/cloudfoundry/multiapps/controller/web/upload/client/DeployFromUrlRemoteClientTest.java
a26345c to
e0c63ee
Compare
|
| private HttpResponse<InputStream> followRedirectsWithValidation(String url, String jobId, UserCredentials userCredentials, | ||
| int redirectCount) throws Exception { | ||
| if (redirectCount > MAX_REDIRECTS) { | ||
| throw new SLException(MessageFormat.format(Messages.ERROR_FROM_REMOTE_MTAR_ENDPOINT_FOR_JOB_WITH_ID, | ||
| getMaskedUri(urlDecodeUrl(url)), Messages.TOO_MANY_REDIRECTS, "", jobId)); | ||
| } | ||
| HttpRequest request = buildFetchFileRequest(url, userCredentials); | ||
| HttpResponse<InputStream> response = httpClient.send(request, HttpResponse.BodyHandlers.ofInputStream()); | ||
| int status = response.statusCode(); | ||
| if (isRedirect(status)) { | ||
| return followRedirect(response, url, jobId, userCredentials, redirectCount); | ||
| } | ||
| if (status < 200 || status >= 300) { | ||
| throwErrorResponseException(response, url, status, jobId); | ||
| throwErrorResponseException(response, url, status, jobId, userCredentials); | ||
| } | ||
| return response; | ||
| } |
There was a problem hiding this comment.
Each redirect re-sends the token to the new target using the same userCredentials, and nothing checks whether the redirect host matches the original. As a result, a redirect to any public host still receives the Authorization: Bearer header.
Could we drop the Authorization header when a redirect crosses hosts and only preserve it for same-host redirects? Will this break this scenario?
| return response; | ||
| } | ||
| @Test | ||
| void downloadFileFromUrlWithTokenSendsBearer() throws Exception { |
There was a problem hiding this comment.
This checks the Authorization header on a single request. The existing redirectToPublicAddressIsFollowed verifies the hops are validated but never inspects the header after a redirect, so nothing covers which credentials reach the second hop. Could we add a cross-host redirect case that asserts that?
| if (StringUtils.isNotBlank(userCredentials.getToken())) { | ||
| return new BearerTokenAuthenticator(userCredentials.getToken()); | ||
| } | ||
| return new BasicAuthenticator(userCredentials.getUsername(), userCredentials.getPassword()); | ||
| } |
There was a problem hiding this comment.
The token branch guards with isNotBlank, but Basic doesn't. Since the fields are @Nullable, could Basic fall through to no-auth when username/password are blank, like the token branch does?
| if (status == HttpStatus.FORBIDDEN.value()) { | ||
| throw new SLException(MessageFormat.format(Messages.DEPLOY_FROM_URL_FORBIDDEN, | ||
| UriUtil.stripUserInfo(url))); | ||
| } | ||
| if (status == HttpStatus.NOT_FOUND.value() && hasToken(userCredentials)) { | ||
| throw new SLException(MessageFormat.format(Messages.DEPLOY_FROM_URL_NOT_FOUND_WITH_TOKEN, | ||
| UriUtil.stripUserInfo(url))); | ||
| } |
There was a problem hiding this comment.
The 404 branch checks hasToken before mentioning the token, but the 403 branch doesn't. A 403 with no token still says "make sure the provided token has the required permissions." Could we guard the 403 message with hasToken too?
| public static final String DEPLOY_FROM_URL_FORBIDDEN = "Access to {0} is forbidden. Make sure the provided token has the required permissions."; | ||
| public static final String DEPLOY_FROM_URL_NOT_FOUND_WITH_TOKEN = "The resource at {0} was not found. If it is private, verify the provided token is valid and has read access."; |
There was a problem hiding this comment.
The other deploy-from-URL errors append the job id for traceability, but these two drop it. Could we keep the pattern so the failures can be correlated in the logs?
| } | ||
|
|
||
| @Test | ||
| void downloadFileFromUrlForbiddenWithTokenReturnsCorrectError() throws Exception { |
There was a problem hiding this comment.
These three error tests repeat the same status->body->send stub. A small stubErrorResponse(status, body) would fold that together, the way stubSuccessfulHttpResponse already does for the 200 case.
| } | ||
| @Test |
No description provided.